Conversation
- Major rewrite of critial systems - EF migrations - Project organisation
There was a problem hiding this comment.
Pull request overview
Implements passkey (FIDO2/WebAuthn) authentication and restructures the API around ASP.NET Core DI + EF Core repositories, replacing the legacy storage layer and introducing centralized CORS/OPTIONS handling.
Changes:
- Added passkey registration/authentication endpoints and persistence (FIDO2 +
UserPasskeystable). - Replaced legacy
IStorageService(file/MySQL helpers) with EF Core (SerbleDbContext) and repository interfaces/implementations. - Introduced DI-based services (token/captcha/anti-spam/email confirmation) plus a middleware-based CORS + automatic OPTIONS responder.
Reviewed changes
Copilot reviewed 100 out of 103 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| migrate.py | New one-off MySQL-to-MySQL migration script for moving legacy tables into the new schema. |
| appsettings.json | Adds configuration sections for DB, API, Stripe, Passkey, Email, JWT, Turnstile. |
| appsettings.Development.json | Removed development-only logging config. |
| Services/Impl/TurnstileCaptchaService.cs | Converts Turnstile verification into DI service using IOptions<TurnstileSettings>. |
| Services/Impl/TokenService.cs | Replaces static token helper with DI-based JWT token service and repository-backed user lookup. |
| Services/Impl/GoogleReCaptchaService.cs | Converts reCAPTCHA verification into DI service using IOptions<ReCaptchaSettings>. |
| Services/Impl/EmailConfirmationService.cs | New DI email-confirmation sender using Email + token service + API settings. |
| Services/Impl/AntiSpamService.cs | Refactors anti-spam to consume a header model + injected captcha services and settings. |
| Services/ITurnstileCaptchaService.cs | New service contract for Turnstile verification. |
| Services/ITokenService.cs | New service contract for JWT issuance/validation flows. |
| Services/IGoogleReCaptchaService.cs | New service contract for reCAPTCHA verification. |
| Services/IEmailConfirmationService.cs | New service contract for email confirmation sending. |
| Services/IAntiSpamService.cs | New service contract for anti-spam validation. |
| SerbleAPI.csproj | Upgrades to .NET 8, adds FIDO2 + EF Core (Pomelo) dependencies, updates JWT package. |
| Repositories/Impl/UserRepository.cs | New EF Core-backed user + authorized-app persistence. |
| Repositories/Impl/ProductRepository.cs | New EF Core-backed owned-products persistence. |
| Repositories/Impl/PasskeyRepository.cs | New EF Core-backed passkey persistence/mapping. |
| Repositories/Impl/NoteRepository.cs | New EF Core-backed user notes persistence. |
| Repositories/Impl/KvRepository.cs | New EF Core-backed KV persistence. |
| Repositories/Impl/AppRepository.cs | New EF Core-backed OAuth app persistence. |
| Repositories/IUserRepository.cs | New repository contract for users + authorized apps. |
| Repositories/IProductRepository.cs | New repository contract for owned products. |
| Repositories/IPasskeyRepository.cs | New repository contract for passkeys. |
| Repositories/INoteRepository.cs | New repository contract for notes. |
| Repositories/IKvRepository.cs | New repository contract for KV. |
| Repositories/IAppRepository.cs | New repository contract for OAuth apps. |
| Program.cs | Rewrites startup: binds options, registers services/repos, configures authZ policies + FIDO2 + EF Core + middleware. |
| Models/SerbleDbContext.cs | New EF Core DbContext with DbSets for all new tables. |
| Models/DbUserPasskey.cs | New EF Core entity for user passkeys. |
| Models/DbUserNote.cs | New EF Core entity for user notes. |
| Models/DbUserAuthorizedApp.cs | New EF Core entity for authorized apps. |
| Models/DbUser.cs | New EF Core entity for users. |
| Models/DbOwnedProduct.cs | New EF Core entity for owned products. |
| Models/DbKv.cs | New EF Core entity for KV store. |
| Models/DbApp.cs | New EF Core entity for OAuth apps. |
| Migrations/SerbleDbContextModelSnapshot.cs | EF Core model snapshot for initial schema. |
| Migrations/20260220115153_Initial.cs | Initial EF Core migration creating tables and indexes. |
| Migrations/20260220115153_Initial.Designer.cs | EF Core migration designer output for the initial schema. |
| Data/Storage/MySQL/MySqlVault.cs | Removes legacy MySQL storage implementation (notes). |
| Data/Storage/MySQL/MySqlUsers.cs | Removes legacy MySQL storage implementation (users). |
| Data/Storage/MySQL/MySqlStorageService.cs | Removes legacy MySQL storage bootstrap/table creation. |
| Data/Storage/MySQL/MySqlProducts.cs | Removes legacy MySQL storage implementation (products). |
| Data/Storage/MySQL/MySqlOAuthApps.cs | Removes legacy MySQL storage implementation (OAuth apps). |
| Data/Storage/MySQL/MySqlKV.cs | Removes legacy MySQL storage implementation (KV). |
| Data/Storage/MySQL/MySqlAuthorizedApps.cs | Removes legacy MySQL storage implementation (authorized apps). |
| Data/Storage/IStorageService.cs | Removes legacy storage abstraction in favor of repositories. |
| Data/Storage/FileStorageService.cs | Removes legacy file-backed storage implementation. |
| Data/ServicesStatusService.cs | Removes legacy logging dependency and simplifies config error handling. |
| Data/SerbleUtils.cs | Adds hashing + enum helpers + multi-dimensional array serialization helpers. |
| Data/Schemas/User.cs | Refactors User to use injected repositories via WithRepos and updates hashing. |
| Data/Schemas/SavedPasskey.cs | New schema for stored passkey material. |
| Data/Raw/RawDataManager.cs | Removes legacy logging dependency during raw-data load. |
| Data/ProductManager.cs | Refactors owned-product lookup to use IProductRepository instead of legacy storage. |
| Data/LocalisationHandler.cs | Removes legacy logging dependency around translation loading. |
| Data/EmailConfirmationService.cs | Removes legacy static email-confirmation helper in favor of DI service. |
| Data/Email.cs | Refactors Email sending to use injected EmailSettings and ILogger. |
| Data/ApiDataSchemas/SerbleAuthorizationHeader.cs | Removes legacy header parsing class, leaving only the enum. |
| Data/ApiDataSchemas/AuthorizationHeaderUser.cs | Removes legacy header parsing class. |
| Data/ApiDataSchemas/AuthorizationHeaderApp.cs | Removes legacy header parsing class. |
| Data/ApiDataSchemas/AntiSpamHeader.cs | New header model for anti-spam header binding. |
| Data/ApiDataSchemas/AccountEditRequest.cs | Refactors account edits to use injected IUserRepository instead of legacy storage/logging. |
| Config/TurnstileSettings.cs | New options model for Turnstile settings. |
| Config/StripeSettings.cs | New options model for Stripe settings. |
| Config/ReCaptchaSettings.cs | New options model for reCAPTCHA settings. |
| Config/PasskeySettings.cs | New options model for FIDO2/WebAuthn server/origin configuration. |
| Config/JwtSettings.cs | New options model for JWT configuration. |
| Config/EmailSettings.cs | New options model for SMTP and from-address configuration. |
| Config/ApiSettings.cs | New options model for bind URL, website URL, redirects, and anti-spam bypass toggle. |
| Config/ApiEmailAddresses.cs | New options model for system/newsletter/contact from-addresses. |
| Authentication/SerbleClaimsPrincipalExtensions.cs | New helpers for reading Serble-specific claims/scopes and loading users. |
| Authentication/SerbleAuthenticationHandler.cs | New custom auth handler supporting SerbleAuth and Authorization: Bearer headers. |
| API/v1/Vault/NotesController.cs | Moves auth/scopes to [Authorize] policies and uses repositories. |
| API/v1/ServicesController.cs | Removes explicit OPTIONS endpoints (now handled centrally). |
| API/v1/Services/Redirect.cs | Removes explicit OPTIONS endpoints (now handled centrally). |
| API/v1/Services/ReCaptchaController.cs | Uses DI reCAPTCHA service; removes explicit OPTIONS. |
| API/v1/Services/RawDataController.cs | Removes explicit OPTIONS. |
| API/v1/Services/CheckUser.cs | Removes explicit OPTIONS. |
| API/v1/RootController.cs | Removes explicit OPTIONS. |
| API/v1/People/TristanController.cs | Adds a new “People” controller with multiple HTTP verbs. |
| API/v1/People/README.md | Minor formatting tweak. |
| API/v1/People/DanController.cs | Removes explicit Allow header manipulation in OPTIONS. |
| API/v1/People/AdamController.cs | Removes explicit Allow header manipulation in OPTIONS. |
| API/v1/Payments/StripeWebhookController.cs | Refactors webhook handling to DI repos/settings and updated Email usage. |
| API/v1/Payments/ProductsController.cs | Uses [Authorize] + repo access for owned products. |
| API/v1/Payments/OrderSuccessController.cs | Moves redirect target to ApiSettings.WebsiteUrl and uses ILogger. |
| API/v1/Payments/CreateCheckoutController.cs | Moves auth to policies, uses DI settings + token service, refactors checkout flows. |
| API/v1/Apps/AppController.cs | Moves auth/scopes to policies and uses repositories. |
| API/v1/Account/PasskeyController.cs | Adds passkey credential creation endpoints and passkey CRUD. |
| API/v1/Account/OAuthTokenController.cs | Uses DI token service + repositories for OAuth flows. |
| API/v1/Account/OAuthAuthController.cs | Redirect target now comes from ApiSettings.WebsiteUrl. |
| API/v1/Account/MfaController.cs | Uses DI token service + policies for MFA endpoints. |
| API/v1/Account/EmailConfirmationController.cs | Uses DI token service + API settings for email confirm redirect. |
| API/v1/Account/AuthorizedAppsController.cs | Uses policies + DI repos and token service for authorized app management. |
| API/v1/Account/AuthController.cs | Adds password auth endpoint and passkey assertion flow with cached challenges. |
| API/v1/Account/AccountProductsController.cs | Uses policy-based auth and repo-based product lookup. |
| API/v1/Account/AccountController.cs | Uses policy-based auth, anti-spam service, repos, and DI email confirmation. |
| API/SerbleCorsMiddleware.cs | New centralized middleware for CORS + automatic OPTIONS handling. |
| API/RedirectsMiddleware.cs | New middleware implementing config-driven redirects. |
| API/Redirects/README.md | Removes old redirects documentation (redirects now config-driven). |
| API/Redirects/Discord.cs | Removes old controller-based redirect implementation. |
| API/Redirects/Adam.cs | Removes old controller-based redirect implementation. |
| API/ControllerManager.cs | Removes per-controller CORS logic and switches request logging to ILogger. |
| .gitignore | Ignores appsettings.Development.json and .idea/ directory. |
Files not reviewed (1)
- Migrations/20260220115153_Initial.Designer.cs: Language not supported
Comments suppressed due to low confidence (2)
Services/Impl/TurnstileCaptchaService.cs:21
HttpClientis instantiated per request and never disposed; over time this can lead to socket exhaustion. AlsoEncoding.Defaultis locale-dependent for JSON payloads. Prefer injecting anHttpClientviaIHttpClientFactory(typed client) and use UTF-8 when creating the JSONStringContent.
Services/Impl/GoogleReCaptchaService.cs:19HttpClientis instantiated per request and never disposed, which can lead to socket exhaustion under load. Prefer injecting anHttpClientviaIHttpClientFactory(typed/named client) and reuse it for requests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+50
to
+65
| public static string StringifyMda(this byte[][] arr) { | ||
| StringBuilder sb = new(); | ||
| foreach (byte[] bytes in arr) { | ||
| sb.Append(bytes.Base64Encode()); | ||
| sb.Append(','); | ||
| } | ||
| return sb.ToString(); | ||
| } | ||
|
|
||
| public static byte[][] ParseMda(this string str, Func<string, byte[]> parse) { | ||
| string[] split = str.Split(','); | ||
| byte[][] arr = new byte[split.Length][]; | ||
| for (int i = 0; i < split.Length; i++) { | ||
| arr[i] = parse(split[i]); | ||
| } | ||
| return arr; |
There was a problem hiding this comment.
StringifyMda appends a trailing comma, but ParseMda splits on ',' without removing empty entries. This will produce a final empty element and will throw when parse("") is called (e.g., Convert.FromBase64String("")). Either avoid the trailing delimiter or use StringSplitOptions.RemoveEmptyEntries (and consider handling empty strings defensively).
Comment on lines
23
to
30
| public async Task<IActionResult> StripeWebhookCallback() { | ||
| string json = await new StreamReader(HttpContext.Request.Body).ReadToEndAsync(); | ||
| string endpointSecret = Program.Testing ? Program.Config!["stripe_testing_webhook_secret"] : Program.Config!["stripe_webhook_secret"]; | ||
| string endpointSecret = settings.Value.ApiKey; | ||
| try { | ||
| Event? stripeEvent = EventUtility.ParseEvent(json); | ||
| StringValues signatureHeader = Request.Headers["Stripe-Signature"]; | ||
| stripeEvent = EventUtility.ConstructEvent(json, | ||
| signatureHeader, endpointSecret); | ||
| stripeEvent = EventUtility.ConstructEvent(json, signatureHeader, endpointSecret); | ||
| bool liveMode = stripeEvent.Livemode; |
There was a problem hiding this comment.
Stripe webhook signature verification must use the webhook signing secret, not the Stripe API key. endpointSecret is currently set to settings.Value.ApiKey, which will cause signature validation failures (and is the wrong secret to use). Switch to settings.Value.WebhookSecret (and optionally validate it is present at startup).
Comment on lines
+45
to
+51
| builder.Services.AddOptions<ApiSettings>().Bind(builder.Configuration.GetSection("Api")); | ||
| builder.Services.AddOptions<StripeSettings>().Bind(builder.Configuration.GetSection("Stripe")); | ||
| builder.Services.AddOptions<PasskeySettings>().Bind(builder.Configuration.GetSection("Passkey")); | ||
| builder.Services.AddOptions<EmailSettings>().Bind(builder.Configuration.GetSection("Email")); | ||
| builder.Services.AddOptions<ReCaptchaSettings>().Bind(builder.Configuration.GetSection("ReCaptcha")); | ||
| builder.Services.AddOptions<JwtSettings>().Bind(builder.Configuration.GetSection("Jwt")); | ||
| builder.Services.AddOptions<TurnstileSettings>().Bind(builder.Configuration.GetSection("Turnstile")); |
There was a problem hiding this comment.
ReCaptchaSettings is bound from the ReCaptcha configuration section, but appsettings.json in this PR does not define a ReCaptcha section. This will leave settings.Value.SecretKey null and break recaptcha verification when used. Either add the ReCaptcha section to config (recommended) or remove/guard the binding and service registration if recaptcha is no longer supported.
Comment on lines
+9
to
+53
| "ConnectionStrings": { | ||
| "MySql": "Server=localhost;Port=3307;Database=serbleapi;User=root;Password=admin;" | ||
| }, | ||
| "Api": { | ||
| "BindUrl": "http://*:5000", | ||
| "WebsiteUrl": "https://localhost:7244", | ||
| "LiveUrl": "http://localhost:5000/", | ||
| "Redirects": { | ||
| "discord": "https://discord.gg/fzvcNhW" | ||
| } | ||
| }, | ||
| "Stripe": { | ||
| "ApiKey": "somestripeapikey", | ||
| "WebhookSecret": "somestripesecret", | ||
| "PremiumSubscriptionId": "somepriceid" | ||
| }, | ||
| "Passkey": { | ||
| "RelyingPartyId": "localhost", | ||
| "RelyingPartyName": "Serble", | ||
| "AllowedOrigins": [ | ||
| "https://serble.net", | ||
| "https://www.serble.net", | ||
| "https://serble", | ||
| "http://localhost:5173" | ||
| ] | ||
| }, | ||
| "Email": { | ||
| "SmtpHost": "smtp.example.com", | ||
| "SmtpPort": 587, | ||
| "SmtpUsername": "system@serble.net", | ||
| "SmtpPassword": "somepassword", | ||
| "Addresses": { | ||
| "System": "system@serble.net", | ||
| "Newsletter": "newsletter@serble.net", | ||
| "Contact": "admin@serble.net" | ||
| } | ||
| }, | ||
| "Jwt": { | ||
| "Issuer": "CoPokBl", | ||
| "Audience": "Privileged Users", | ||
| "Secret": "somejwtsecretkey" | ||
| }, | ||
| "Turnstile": { | ||
| "SecretKey": "somecaptchasecretkey" | ||
| } |
There was a problem hiding this comment.
This file commits multiple plaintext secrets/credentials (DB password, Stripe keys, SMTP password, JWT secret, captcha secret). Even if placeholders, this pattern makes it easy to accidentally ship real secrets and encourages storing credentials in git. Replace these values with empty placeholders and load real secrets from environment variables / user-secrets / secret manager, and consider adding an appsettings.example.json for documentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement passkey support and rewrite the majority of the project in order for it to be more organised.